Browse Source

Merge pull request #11271 from growilabs/fix/11263-lsx-disabled-on-share-link-page

fix: disable $lsx() and attachment-refs directives on share link pages to prevent Basic auth challenge
mergify[bot] 2 days ago
parent
commit
0f89e3f619

+ 12 - 3
apps/app/src/client/services/renderer/renderer.tsx

@@ -112,7 +112,10 @@ export const generateViewOptions = (
       lsxGrowiDirective.rehypePlugin,
       { pagePath, isSharedPage: config.isSharedPage },
     ],
-    [refsGrowiDirective.rehypePlugin, { pagePath }],
+    [
+      refsGrowiDirective.rehypePlugin,
+      { pagePath, isSharedPage: config.isSharedPage },
+    ],
     rehypeSanitizePlugin,
     katex,
     [relocateToc.rehypePluginStore, { storeTocNode }],
@@ -238,7 +241,10 @@ export const generateSimpleViewOptions = (
       lsxGrowiDirective.rehypePlugin,
       { pagePath, isSharedPage: config.isSharedPage },
     ],
-    [refsGrowiDirective.rehypePlugin, { pagePath }],
+    [
+      refsGrowiDirective.rehypePlugin,
+      { pagePath, isSharedPage: config.isSharedPage },
+    ],
     [keywordHighlighter.rehypePlugin, { keywords: highlightKeywords }],
     rehypeSanitizePlugin,
     katex,
@@ -342,7 +348,10 @@ export const generatePreviewOptions = (
       lsxGrowiDirective.rehypePlugin,
       { pagePath, isSharedPage: config.isSharedPage },
     ],
-    [refsGrowiDirective.rehypePlugin, { pagePath }],
+    [
+      refsGrowiDirective.rehypePlugin,
+      { pagePath, isSharedPage: config.isSharedPage },
+    ],
     addLineNumberAttribute.rehypePlugin,
     rehypeSanitizePlugin,
     katex,

+ 21 - 0
apps/app/src/pages/general-page/configuration-props.ts

@@ -125,3 +125,24 @@ export const getServerSideGeneralPageProps: GetServerSideProps<
     },
   };
 };
+
+export const getServerSideShareLinkRendererConfigProps: GetServerSideProps<
+  RendererConfigProps
+> = async (context: GetServerSidePropsContext) => {
+  const result = await getServerSideRendererConfigProps(context);
+
+  if ('props' in result) {
+    const props = await result.props;
+    return {
+      props: {
+        ...props,
+        rendererConfig: {
+          ...props.rendererConfig,
+          isSharedPage: true,
+        },
+      },
+    };
+  }
+
+  return result;
+};

+ 1 - 0
apps/app/src/pages/general-page/index.ts

@@ -1,6 +1,7 @@
 export {
   getServerSideGeneralPageProps,
   getServerSideRendererConfigProps,
+  getServerSideShareLinkRendererConfigProps,
 } from './configuration-props';
 export { isValidGeneralPageInitialProps } from './type-guards';
 export type * from './types';

+ 2 - 2
apps/app/src/pages/share/[[...path]]/server-side-props.ts

@@ -13,7 +13,7 @@ import {
 } from '../../common-props';
 import {
   getServerSideGeneralPageProps,
-  getServerSideRendererConfigProps,
+  getServerSideShareLinkRendererConfigProps,
   isValidGeneralPageInitialProps,
 } from '../../general-page';
 import { addActivity } from '../../utils/activity';
@@ -58,7 +58,7 @@ export async function getServerSidePropsForInitial(
     getServerSideCommonEachProps(context),
     getServerSideCommonInitialProps(context),
     getServerSideGeneralPageProps(context),
-    getServerSideRendererConfigProps(context),
+    getServerSideShareLinkRendererConfigProps(context),
     getServerSideI18nProps(context, ['translation']),
     getPageDataForInitial(context),
   ]);

+ 30 - 0
packages/remark-attachment-refs/src/client/components/AttachmentRefsDisabled.tsx

@@ -0,0 +1,30 @@
+import React, { type JSX } from 'react';
+
+type Props = {
+  /** The directive name to show in the message (e.g. 'ref', 'refs', 'gallery') */
+  name: string;
+};
+
+/**
+ * Placeholder rendered in place of a ref/refs directive on a share link page.
+ *
+ * Attachment ref directives fetch from `/_api/attachment-refs/*`, which is not
+ * accessible to anonymous share-link viewers. Rendering this instead of the
+ * substance avoids issuing those unauthenticated requests (see issue #11263).
+ */
+export const AttachmentRefsDisabled = React.memo(
+  ({ name }: Props): JSX.Element => {
+    return (
+      <div className="text-muted">
+        <span
+          className="material-symbols-outlined fs-5 me-1"
+          aria-hidden="true"
+        >
+          info
+        </span>
+        <small>{name} is not available on the share link page</small>
+      </div>
+    );
+  },
+);
+AttachmentRefsDisabled.displayName = 'AttachmentRefsDisabled';

+ 5 - 5
packages/remark-attachment-refs/src/client/components/Gallery.tsx

@@ -1,5 +1,6 @@
 import React, { type JSX } from 'react';
 
+import { AttachmentRefsDisabled } from './AttachmentRefsDisabled';
 import type { Props } from './RefsImg';
 import { RefsImgSubstance } from './RefsImg';
 
@@ -7,6 +8,9 @@ const gridDefault = 'col-4';
 const gridGapDefault = '1px';
 
 export const Gallery = React.memo((props: Props): JSX.Element => {
+  if (props.isSharedPage) {
+    return <AttachmentRefsDisabled name="gallery" />;
+  }
   const grid = props.grid || gridDefault;
   const gridGap = props.gridGap || gridGapDefault;
   return <RefsImgSubstance grid={grid} gridGap={gridGap} {...props} />;
@@ -14,11 +18,7 @@ export const Gallery = React.memo((props: Props): JSX.Element => {
 
 export const GalleryImmutable = React.memo(
   (props: Omit<Props, 'isImmutable'>): JSX.Element => {
-    const grid = props.grid || gridDefault;
-    const gridGap = props.gridGap || gridGapDefault;
-    return (
-      <RefsImgSubstance grid={grid} gridGap={gridGap} {...props} isImmutable />
-    );
+    return <Gallery {...props} isImmutable />;
   },
 );
 

+ 6 - 1
packages/remark-attachment-refs/src/client/components/Ref.tsx

@@ -2,12 +2,14 @@ import React, { type JSX, useMemo } from 'react';
 
 import { useSWRxRef } from '../stores/refs';
 import { AttachmentList } from './AttachmentList';
+import { AttachmentRefsDisabled } from './AttachmentRefsDisabled';
 import { RefsContext } from './util/refs-context';
 
 type Props = {
   fileNameOrId: string;
   pagePath: string;
   isImmutable?: boolean;
+  isSharedPage?: boolean;
 };
 
 const RefSubstance = React.memo(
@@ -35,12 +37,15 @@ const RefSubstance = React.memo(
 );
 
 export const Ref = React.memo((props: Props): JSX.Element => {
+  if (props.isSharedPage) {
+    return <AttachmentRefsDisabled name="ref" />;
+  }
   return <RefSubstance {...props} />;
 });
 
 export const RefImmutable = React.memo(
   (props: Omit<Props, 'isImmutable'>): JSX.Element => {
-    return <RefSubstance {...props} isImmutable />;
+    return <Ref {...props} isImmutable />;
   },
 );
 

+ 6 - 1
packages/remark-attachment-refs/src/client/components/RefImg.tsx

@@ -2,6 +2,7 @@ import React, { type JSX, useMemo } from 'react';
 
 import { useSWRxRef } from '../stores/refs';
 import { AttachmentList } from './AttachmentList';
+import { AttachmentRefsDisabled } from './AttachmentRefsDisabled';
 import { RefsContext } from './util/refs-context';
 
 type Props = {
@@ -14,6 +15,7 @@ type Props = {
   alt?: string;
 
   isImmutable?: boolean;
+  isSharedPage?: boolean;
 };
 
 const RefImgSubstance = React.memo(
@@ -58,12 +60,15 @@ const RefImgSubstance = React.memo(
 );
 
 export const RefImg = React.memo((props: Props): JSX.Element => {
+  if (props.isSharedPage) {
+    return <AttachmentRefsDisabled name="refimg" />;
+  }
   return <RefImgSubstance {...props} />;
 });
 
 export const RefImgImmutable = React.memo(
   (props: Omit<Props, 'isImmutable'>): JSX.Element => {
-    return <RefImgSubstance {...props} isImmutable />;
+    return <RefImg {...props} isImmutable />;
   },
 );
 

+ 6 - 1
packages/remark-attachment-refs/src/client/components/Refs.tsx

@@ -2,6 +2,7 @@ import React, { type JSX, useMemo } from 'react';
 
 import { useSWRxRefs } from '../stores/refs';
 import { AttachmentList } from './AttachmentList';
+import { AttachmentRefsDisabled } from './AttachmentRefsDisabled';
 import { RefsContext } from './util/refs-context';
 
 type Props = {
@@ -11,6 +12,7 @@ type Props = {
   regexp?: string;
 
   isImmutable?: boolean;
+  isSharedPage?: boolean;
 };
 
 const RefsSubstance = React.memo(
@@ -51,12 +53,15 @@ const RefsSubstance = React.memo(
 );
 
 export const Refs = React.memo((props: Props): JSX.Element => {
+  if (props.isSharedPage) {
+    return <AttachmentRefsDisabled name="refs" />;
+  }
   return <RefsSubstance {...props} />;
 });
 
 export const RefsImmutable = React.memo(
   (props: Omit<Props, 'isImmutable'>): JSX.Element => {
-    return <RefsSubstance {...props} isImmutable />;
+    return <Refs {...props} isImmutable />;
   },
 );
 

+ 6 - 1
packages/remark-attachment-refs/src/client/components/RefsImg.tsx

@@ -2,6 +2,7 @@ import React, { type JSX, useMemo } from 'react';
 
 import { useSWRxRefs } from '../stores/refs';
 import { AttachmentList } from './AttachmentList';
+import { AttachmentRefsDisabled } from './AttachmentRefsDisabled';
 import { RefsContext } from './util/refs-context';
 
 export type Props = {
@@ -19,6 +20,7 @@ export type Props = {
   noCarousel?: string;
 
   isImmutable?: boolean;
+  isSharedPage?: boolean;
 };
 
 export const RefsImgSubstance = React.memo(
@@ -109,12 +111,15 @@ export const RefsImgSubstance = React.memo(
 );
 
 export const RefsImg = React.memo((props: Props): JSX.Element => {
+  if (props.isSharedPage) {
+    return <AttachmentRefsDisabled name="refsimg" />;
+  }
   return <RefsImgSubstance {...props} />;
 });
 
 export const RefsImgImmutable = React.memo(
   (props: Omit<Props, 'isImmutable'>): JSX.Element => {
-    return <RefsImgSubstance {...props} isImmutable />;
+    return <RefsImg {...props} isImmutable />;
   },
 );
 

+ 69 - 0
packages/remark-attachment-refs/src/client/components/share-link-guard.spec.tsx

@@ -0,0 +1,69 @@
+import { render, screen } from '@testing-library/react';
+
+import { useSWRxRef, useSWRxRefs } from '../stores/refs';
+import { Gallery } from './Gallery';
+import { Ref } from './Ref';
+import { RefImg } from './RefImg';
+import { Refs } from './Refs';
+import { RefsImg } from './RefsImg';
+
+// The contract under test is "no request is issued on a share link page".
+// Mock the data-fetching hooks so we can assert they are never called,
+// and stub AttachmentList so the test does not pull in @growi/ui.
+vi.mock('../stores/refs', () => ({
+  useSWRxRef: vi.fn(() => ({
+    data: undefined,
+    error: undefined,
+    isLoading: false,
+  })),
+  useSWRxRefs: vi.fn(() => ({ data: [], error: undefined, isLoading: false })),
+}));
+
+vi.mock('./AttachmentList', () => ({
+  AttachmentList: () => <div data-testid="attachment-list" />,
+}));
+
+const DISABLED_MESSAGE = /not available on the share link page/i;
+
+describe('attachment-refs directives on a share link page', () => {
+  describe('when isSharedPage is true', () => {
+    it.each`
+      label        | mount
+      ${'ref'}     | ${() => render(<Ref pagePath="/foo" fileNameOrId="a.png" isSharedPage />)}
+      ${'refimg'}  | ${() => render(<RefImg pagePath="/foo" fileNameOrId="a.png" isSharedPage />)}
+      ${'refs'}    | ${() => render(<Refs pagePath="/foo" isSharedPage />)}
+      ${'refsimg'} | ${() => render(<RefsImg pagePath="/foo" isSharedPage />)}
+      ${'gallery'} | ${() => render(<Gallery pagePath="/foo" isSharedPage />)}
+    `(
+      'shows the disabled message and issues no request for $label',
+      ({ mount }: { mount: () => void }) => {
+        mount();
+
+        expect(screen.queryByText(DISABLED_MESSAGE)).not.toBeNull();
+        expect(screen.queryByTestId('attachment-list')).toBeNull();
+        expect(useSWRxRef).not.toHaveBeenCalled();
+        expect(useSWRxRefs).not.toHaveBeenCalled();
+      },
+    );
+  });
+
+  describe('when isSharedPage is not set', () => {
+    it.each`
+      label        | mount                                                            | hook
+      ${'ref'}     | ${() => render(<Ref pagePath="/foo" fileNameOrId="a.png" />)}    | ${useSWRxRef}
+      ${'refimg'}  | ${() => render(<RefImg pagePath="/foo" fileNameOrId="a.png" />)} | ${useSWRxRef}
+      ${'refs'}    | ${() => render(<Refs pagePath="/foo" />)}                        | ${useSWRxRefs}
+      ${'refsimg'} | ${() => render(<RefsImg pagePath="/foo" />)}                     | ${useSWRxRefs}
+      ${'gallery'} | ${() => render(<Gallery pagePath="/foo" />)}                     | ${useSWRxRefs}
+    `(
+      'renders attachments and requests data for $label',
+      ({ mount, hook }: { mount: () => void; hook: () => unknown }) => {
+        mount();
+
+        expect(screen.queryByText(DISABLED_MESSAGE)).toBeNull();
+        expect(screen.queryByTestId('attachment-list')).not.toBeNull();
+        expect(hook).toHaveBeenCalled();
+      },
+    );
+  });
+});

+ 60 - 0
packages/remark-attachment-refs/src/client/services/renderer/refs.spec.ts

@@ -0,0 +1,60 @@
+import { rehypePlugin } from './refs';
+
+type RefTagName = 'ref' | 'refimg' | 'refs' | 'refsimg' | 'gallery';
+
+const createTree = (
+  tagName: RefTagName,
+  properties: Record<string, unknown> = {},
+) => ({
+  type: 'root',
+  children: [
+    {
+      type: 'element',
+      tagName,
+      properties,
+      children: [],
+    },
+  ],
+});
+
+const runRehype = (
+  tree: ReturnType<typeof createTree>,
+  options: { pagePath?: string; isSharedPage?: boolean },
+): void => {
+  // WHY: unified's `Plugin` type is not directly callable; narrow it to the
+  // (options) => (tree) => void shape this factory plugin actually has.
+  (rehypePlugin as (opts: typeof options) => (tree: unknown) => void)(options)(
+    tree,
+  );
+};
+
+const getProperties = (
+  tree: ReturnType<typeof createTree>,
+): Record<string, unknown> =>
+  (tree.children[0] as { properties: Record<string, unknown> }).properties;
+
+const tags: RefTagName[] = ['ref', 'refimg', 'refs', 'refsimg', 'gallery'];
+
+describe('refs rehypePlugin - isSharedPage injection', () => {
+  it.each(
+    tags,
+  )('injects isSharedPage=true into <%s> when the option is set', (tagName) => {
+    const tree = createTree(tagName);
+    runRehype(tree, { pagePath: '/foo', isSharedPage: true });
+    expect(getProperties(tree).isSharedPage).toBe(true);
+  });
+
+  it.each(
+    tags,
+  )('leaves isSharedPage undefined on <%s> when the option is omitted', (tagName) => {
+    const tree = createTree(tagName);
+    runRehype(tree, { pagePath: '/foo' });
+    expect(getProperties(tree).isSharedPage).toBeUndefined();
+  });
+
+  it('does not override an isSharedPage already present on the element', () => {
+    const tree = createTree('refs', { isSharedPage: false });
+    runRehype(tree, { pagePath: '/foo', isSharedPage: true });
+    expect(getProperties(tree).isSharedPage).toBe(false);
+  });
+});

+ 19 - 6
packages/remark-attachment-refs/src/client/services/renderer/refs.ts

@@ -150,6 +150,7 @@ const resolvePath = (pagePath: string, basePath: string) => {
 
 type RefRehypePluginParams = {
   pagePath?: string;
+  isSharedPage?: boolean;
 };
 
 export const rehypePlugin: Plugin<[RefRehypePluginParams]> = (options = {}) => {
@@ -170,7 +171,15 @@ export const rehypePlugin: Plugin<[RefRehypePluginParams]> = (options = {}) => {
 
     for (const refElem of elements) {
       if (refElem.properties == null) {
-        continue;
+        // Initialize properties if null to avoid errors down the line
+        refElem.properties = {};
+      }
+
+      // Inject isSharedPage from the renderer config unless the element already
+      // declares it, so the directive can disable itself on a share link page.
+      const isSharedPage = refElem.properties.isSharedPage;
+      if (isSharedPage == null || typeof isSharedPage !== 'boolean') {
+        refElem.properties.isSharedPage = options.isSharedPage;
       }
 
       const prefix = refElem.properties.prefix;
@@ -199,13 +208,17 @@ export const rehypePlugin: Plugin<[RefRehypePluginParams]> = (options = {}) => {
   };
 };
 
+// Attributes injected by the renderer (not authored in markdown) that must
+// survive sanitization for every ref directive.
+const COMMON_ATTRIBUTES = ['isSharedPage'];
+
 export const sanitizeOption: SanitizeOption = {
   tagNames: ['ref', 'refimg', 'refs', 'refsimg', 'gallery'],
   attributes: {
-    ref: REF_SUPPORTED_ATTRIBUTES,
-    refimg: REF_IMG_SUPPORTED_ATTRIBUTES,
-    refs: REFS_SUPPORTED_ATTRIBUTES,
-    refsimg: REFS_IMG_SUPPORTED_ATTRIBUTES,
-    gallery: REFS_IMG_SUPPORTED_ATTRIBUTES,
+    ref: [...REF_SUPPORTED_ATTRIBUTES, ...COMMON_ATTRIBUTES],
+    refimg: [...REF_IMG_SUPPORTED_ATTRIBUTES, ...COMMON_ATTRIBUTES],
+    refs: [...REFS_SUPPORTED_ATTRIBUTES, ...COMMON_ATTRIBUTES],
+    refsimg: [...REFS_IMG_SUPPORTED_ATTRIBUTES, ...COMMON_ATTRIBUTES],
+    gallery: [...REFS_IMG_SUPPORTED_ATTRIBUTES, ...COMMON_ATTRIBUTES],
   },
 };

+ 1 - 1
packages/remark-attachment-refs/src/server/routes/refs.ts

@@ -66,7 +66,7 @@ const loginRequiredFallback = (_req, res) => {
   return res.status(403).send('login required');
 };
 
-export const routesFactory = (crowi): Promise<Router> => {
+export const routesFactory = (crowi): Router => {
   const loginRequired = crowi.loginRequiredFactory(
     crowi,
     true,

+ 2 - 0
packages/remark-attachment-refs/vitest.config.ts

@@ -11,6 +11,8 @@ export default defineConfig({
       // Use jsdom for client-side tests
       ['**/client/**/*.spec.ts', 'jsdom'],
       ['**/client/**/*.test.ts', 'jsdom'],
+      ['**/client/**/*.spec.tsx', 'jsdom'],
+      ['**/client/**/*.test.tsx', 'jsdom'],
     ],
   },
 });