diff --git a/libraries/overleaf-editor-core/lib/operation/index.js b/libraries/overleaf-editor-core/lib/operation/index.js index ae7f2bcf01..47b08c37a6 100644 --- a/libraries/overleaf-editor-core/lib/operation/index.js +++ b/libraries/overleaf-editor-core/lib/operation/index.js @@ -35,7 +35,8 @@ class Operation { if ( 'textOperation' in raw || 'commentId' in raw || - 'deleteComment' in raw + 'deleteComment' in raw || + 'noOp' in raw ) { return EditFileOperation.fromRaw(raw) } diff --git a/libraries/overleaf-editor-core/test/unit/edit_file_operation.test.js b/libraries/overleaf-editor-core/test/unit/edit_file_operation.test.js index 1ed1ab4a71..3538c68c79 100644 --- a/libraries/overleaf-editor-core/test/unit/edit_file_operation.test.js +++ b/libraries/overleaf-editor-core/test/unit/edit_file_operation.test.js @@ -78,4 +78,18 @@ describe('EditFileOperation', function () { expect(editFileOperation1).to.deep.equal(edit('foo.tex', ['x'])) }) }) + + describe('fromRaw / toRaw', function () { + it('round-trips an edit no-op', function () { + const editFileOperation = Operation.editFile( + 'foo.tex', + EditOperationBuilder.fromJSON({ noOp: true }) + ) + const raw = editFileOperation.toRaw() + expect(raw).to.eql({ pathname: 'foo.tex', noOp: true }) + + const roundTripped = Operation.fromRaw(raw) + expect(roundTripped).to.deep.equal(editFileOperation) + }) + }) }) diff --git a/libraries/validation-tools/package.json b/libraries/validation-tools/package.json index f951940ce5..0d0c41ee73 100644 --- a/libraries/validation-tools/package.json +++ b/libraries/validation-tools/package.json @@ -24,6 +24,7 @@ "zod-validation-error": "^4.0.1" }, "devDependencies": { + "@overleaf/logger": "workspace:*", "@types/express": "4.17.23", "@types/node": "^24.5.2", "eslint": "^10.4.0", diff --git a/libraries/validation-tools/parseReq.js b/libraries/validation-tools/parseReq.js index 7781866c10..9a313980d8 100644 --- a/libraries/validation-tools/parseReq.js +++ b/libraries/validation-tools/parseReq.js @@ -179,7 +179,12 @@ function sanitizeIssues(issues, depth = 0) { path: issue.path.map(seg => String(seg).slice(0, 64)).join('.'), message: String(issue.message).slice(0, 200), } - if (issue.code === 'invalid_union' && depth < 2) { + // Bounded by schema shape, not user input: each level corresponds to one + // more union nested inside a union member (e.g. rawOperation -> rawFile + // -> rawFileMetadata -> the linked-file provider discriminatedUnion), so + // raising this cannot be exploited to grow log volume from adversarial + // input the way an unbounded path/array traversal could. + if (issue.code === 'invalid_union' && depth < 5) { sanitized.errors = issue.errors.map(nested => sanitizeIssues(nested, depth + 1) ) @@ -222,7 +227,7 @@ function logSchemaFailure(req, schema, kind, issues) { .join('\n') logger.warn( - { location, kind, issues: sanitizedIssues, caller }, + { location, kind, issues: sanitizedIssues, caller, req }, 'req-validation: request failed schema in log-only rollout' ) } diff --git a/libraries/validation-tools/test/unit/src/parseReqLogOnlyRollout.test.ts b/libraries/validation-tools/test/unit/src/parseReqLogOnlyRollout.test.ts index 5cb95c9e30..fb9a241d41 100644 --- a/libraries/validation-tools/test/unit/src/parseReqLogOnlyRollout.test.ts +++ b/libraries/validation-tools/test/unit/src/parseReqLogOnlyRollout.test.ts @@ -8,6 +8,7 @@ import { resetReqValidationLoggingForTests, } from '../../../parseReq' import { zz } from '../../../zodHelpers' +import serializers from '@overleaf/logger/serializers' const RAW_BODY = Symbol.for('overleaf.lockdown.rawBody') const RAW_QUERY = Symbol.for('overleaf.lockdown.rawQuery') @@ -56,6 +57,28 @@ function lockedRequest({ type WarnCall = [Record, string] +// Mirrors what a real bunyan logger does to each field before writing a log +// line -- warnMock only records the raw ctx, so assertions on the logged +// output must run it through the same serializers to see what would actually +// reach the log (e.g. the req serializer drops req.body). +function serializedLogOutput(calls: WarnCall[]) { + return JSON.stringify( + calls.map(([ctx, msg]) => [ + Object.fromEntries( + Object.entries(ctx).map(([key, value]) => [ + key, + key in serializers + ? (serializers as Record unknown>)[key]( + value + ) + : value, + ]) + ), + msg, + ]) + ) +} + describe('parseReq log-only rollout', () => { // This block intentionally runs before anything else in the file calls // setLogger(), so the module-level logger is still unset here. @@ -296,7 +319,7 @@ describe('parseReq log-only rollout', () => { parseReq(req, schema, { logOnly: true }) - expect(JSON.stringify(warnMock.mock.calls)).not.toContain( + expect(serializedLogOutput(warnMock.mock.calls)).not.toContain( 'SECRET_VALUE_123' ) }) @@ -311,7 +334,7 @@ describe('parseReq log-only rollout', () => { parseReq(req, schema, { logOnly: true }) - const logged = JSON.stringify(warnMock.mock.calls) + const logged = serializedLogOutput(warnMock.mock.calls) expect(logged).toContain('extraSecretField') expect(logged).not.toContain('TOP_SECRET_VALUE') }) @@ -325,7 +348,7 @@ describe('parseReq log-only rollout', () => { parseReq(req, schema, { logOnly: true }) - const logged = JSON.stringify(warnMock.mock.calls) + const logged = serializedLogOutput(warnMock.mock.calls) expect(logged).toContain(longKey.slice(0, 64)) expect(logged).not.toContain(longKey) }) @@ -367,10 +390,50 @@ describe('parseReq log-only rollout', () => { expect(Array.isArray(unionIssue.errors)).toBe(true) expect(unionIssue.errors.length).toBeGreaterThan(0) - expect(JSON.stringify(warnMock.mock.calls)).not.toContain( + expect(serializedLogOutput(warnMock.mock.calls)).not.toContain( 'UNION_SECRET_VALUE' ) }) + + it('expands a union nested 3 levels deep inside other unions', () => { + // Mirrors overleaf-editor-core's rawOperation -> rawFile -> + // rawFileMetadata shape: a union of objects, one of whose fields is + // itself a union of objects, one of whose fields is itself a union. + const level3 = z.union([ + z.strictObject({ a: z.literal('a') }), + z.strictObject({ b: z.literal('b') }), + ]) + const level2 = z.union([ + z.strictObject({ inner: level3 }), + z.strictObject({ other: z.string() }), + ]) + const level1 = z.union([ + z.strictObject({ mid: level2 }), + z.strictObject({ different: z.string() }), + ]) + const req = { + body: { value: { mid: { inner: { c: 'nope' } } } }, + } as Request + const schema = z.object({ body: z.object({ value: level1 }) }) + + parseReq(req, schema, { logOnly: true }) + + const [ctx] = warnMock.mock.calls[0] as WarnCall + // Walk down: top union (value) -> level1 branch (mid) -> level2 + // union (inner) -> level3 union, whose own branch errors (unknown + // keys "a"/"b" not satisfied by "c") must have survived. + const top = ctx.issues.find((i: any) => i.code === 'invalid_union') + const level1Errors = top.errors[0] // the "mid" branch + const level2Issue = level1Errors.find( + (i: any) => i.code === 'invalid_union' + ) + const level2Errors = level2Issue.errors[0] // the "inner" branch + const level3Issue = level2Errors.find( + (i: any) => i.code === 'invalid_union' + ) + expect(level3Issue.errors).toBeDefined() + expect(level3Issue.errors.length).toBeGreaterThan(0) + }) }) }) diff --git a/services/web/app/src/Features/Compile/CompileController.mjs b/services/web/app/src/Features/Compile/CompileController.mjs index 9b3debb2d3..7a2000b745 100644 --- a/services/web/app/src/Features/Compile/CompileController.mjs +++ b/services/web/app/src/Features/Compile/CompileController.mjs @@ -583,7 +583,7 @@ const _CompileController = { async proxySyncCode(req, res) { const { file, line, column } = req.query - if (file == null) { + if (!file) { throw new Error('missing file parameter') } // Check that we are dealing with a simple file path (this is not diff --git a/services/web/frontend/extracted-translations.json b/services/web/frontend/extracted-translations.json index 83fac34879..660829b326 100644 --- a/services/web/frontend/extracted-translations.json +++ b/services/web/frontend/extracted-translations.json @@ -2066,6 +2066,7 @@ "syncing": "", "synctex_error_recompile_and_try_again": "", "synctex_failed": "", + "synctex_switch_files_and_try_again": "", "syntax_checks": "", "syntax_validation": "", "tab_connecting": "", diff --git a/services/web/frontend/js/features/pdf-preview/components/synctex-toasts.tsx b/services/web/frontend/js/features/pdf-preview/components/synctex-toasts.tsx index ad0a7f53b9..c37ffe76d3 100644 --- a/services/web/frontend/js/features/pdf-preview/components/synctex-toasts.tsx +++ b/services/web/frontend/js/features/pdf-preview/components/synctex-toasts.tsx @@ -28,6 +28,12 @@ export const SynctexRequestErrorToast = () => { return {t('synctex_error_recompile_and_try_again')} } +export const SynctexNoFileToast = () => { + const { t } = useTranslation() + + return {t('synctex_switch_files_and_try_again')} +} + const generators: GlobalToastGeneratorEntry[] = [ { key: 'synctex:file-error', @@ -49,6 +55,16 @@ const generators: GlobalToastGeneratorEntry[] = [ isDismissible: true, }), }, + { + key: 'synctex:no-file-error', + generator: () => ({ + content: , + type: 'warning', + autoHide: true, + delay: 4000, + isDismissible: true, + }), + }, ] export default generators @@ -72,3 +88,13 @@ export const showSynctexRequestErrorToast = () => { }) ) } + +export const showSynctexNoFileToast = () => { + window.dispatchEvent( + new CustomEvent('ide:show-toast', { + detail: { + key: 'synctex:no-file-error', + }, + }) + ) +} diff --git a/services/web/frontend/js/features/pdf-preview/hooks/use-synctex.ts b/services/web/frontend/js/features/pdf-preview/hooks/use-synctex.ts index 5ef488fc8d..38ceabb0e4 100644 --- a/services/web/frontend/js/features/pdf-preview/hooks/use-synctex.ts +++ b/services/web/frontend/js/features/pdf-preview/hooks/use-synctex.ts @@ -18,11 +18,12 @@ import { isValidTeXFile } from '@/main/is-valid-tex-file' import { PdfScrollPosition } from '@/shared/hooks/use-pdf-scroll-position' import { showFileErrorToast, + showSynctexNoFileToast, showSynctexRequestErrorToast, } from '@/features/pdf-preview/components/synctex-toasts' export default function useSynctex(): { - syncToPdf: () => void + syncToPdf: () => boolean syncToCode: ({ visualOffset }: { visualOffset?: number }) => void syncToPdfInFlight: boolean syncToCodeInFlight: boolean @@ -150,18 +151,25 @@ export default function useSynctex(): { const syncToPdf = useCallback(() => { const file = getCurrentFilePath() + if (!file) { + showSynctexNoFileToast() + return false + } if (cursorPositionRef.current) { const { row, column } = cursorPositionRef.current const params = new URLSearchParams({ - file: file ?? '', + file, line: String(row + 1), column: String(column), }).toString() goToPdfLocation(params) + return true } + + return false }, [getCurrentFilePath, goToPdfLocation]) useEventListener( diff --git a/services/web/frontend/js/features/source-editor/hooks/use-context-menu-items.tsx b/services/web/frontend/js/features/source-editor/hooks/use-context-menu-items.tsx index 612f283965..12d6652aa7 100644 --- a/services/web/frontend/js/features/source-editor/hooks/use-context-menu-items.tsx +++ b/services/web/frontend/js/features/source-editor/hooks/use-context-menu-items.tsx @@ -221,10 +221,14 @@ export const useContextMenuItems = () => { method: 'editor-context-menu', direction: 'code-location-in-pdf', }) - requestedPdfSyncRef.current = true - syncToPdf() - view.focus() - }, [syncToPdf, view, changeLayout, isEditorOnly]) + + if (syncToPdf()) { + requestedPdfSyncRef.current = true + view.focus() + } else { + closeMenu() + } + }, [syncToPdf, view, changeLayout, isEditorOnly, closeMenu]) return { closeMenu, diff --git a/services/web/locales/en.json b/services/web/locales/en.json index a3587ad5a4..fdaab07611 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -2651,6 +2651,7 @@ "syncing": "Syncing", "synctex_error_recompile_and_try_again": "That didn’t work. Recompile and try again.", "synctex_failed": "Couldn’t find the corresponding source file", + "synctex_switch_files_and_try_again": "That didn’t work. Try switching files and try again.", "syntax_checks": "Syntax checks", "syntax_validation": "Code check", "tab_connecting": "Connecting with the editor", diff --git a/services/web/test/frontend/components/pdf-preview/pdf-synctex-controls.spec.tsx b/services/web/test/frontend/components/pdf-preview/pdf-synctex-controls.spec.tsx index 279dc9b533..376a7bb3f2 100644 --- a/services/web/test/frontend/components/pdf-preview/pdf-synctex-controls.spec.tsx +++ b/services/web/test/frontend/components/pdf-preview/pdf-synctex-controls.spec.tsx @@ -1,4 +1,5 @@ import PdfSynctexControls from '../../../../frontend/js/features/pdf-preview/components/pdf-synctex-controls' +import { GlobalToasts } from '../../../../frontend/js/features/ide-react/components/global-toasts' import { cloneDeep } from 'lodash' import { useDetachCompileContext as useCompileContext } from '../../../../frontend/js/shared/context/detach-compile-context' import { useFileTreeData } from '../../../../frontend/js/shared/context/file-tree-data-context' @@ -76,11 +77,11 @@ const WithSelectedEntities = ({ return null } -function mockProviders() { +function mockProviders(currentDocumentId: string | null = '_root_doc_id') { return { EditorOpenDocProvider: makeEditorOpenDocProvider({ openDocName: 'main.tex', - currentDocumentId: null, + currentDocumentId: currentDocumentId as any, currentDocument: { doc_id: 'test-doc', getSnapshot: () => 'some doc content', @@ -162,6 +163,50 @@ describe('', function () { ) }) + it('shows an error toast and does not sync when there is no resolvable file path', function () { + cy.interceptCompile() + + const scope = mockScope() + const providers = mockProviders(null) + + cy.intercept( + { pathname: '/project/*/sync/code' }, + cy.spy().as('syncCodeRequest') + ) + + cy.mount( + + + + + + + ) + + cy.waitForCompile() + + // mock editor cursor position update + cy.window().then(win => { + win.dispatchEvent( + new CustomEvent('cursor:editor:update', { + detail: { row: 100, column: 10 }, + }) + ) + }) + + cy.findByRole('button', { name: 'Go to code location in PDF' }).click() + + cy.findByText( + 'That didn’t work. Try switching files and try again.' + ).should('exist') + + cy.findByRole('button', { name: 'Go to code location in PDF' }).should( + 'not.be.disabled' + ) + + cy.get('@syncCodeRequest').should('not.have.been.called') + }) + it('disables button when multiple entities are selected', function () { cy.interceptCompile() diff --git a/services/web/test/frontend/features/source-editor/components/codemirror-editor-context-menu.spec.tsx b/services/web/test/frontend/features/source-editor/components/codemirror-editor-context-menu.spec.tsx index 16e5afc79f..9edb2d39ab 100644 --- a/services/web/test/frontend/features/source-editor/components/codemirror-editor-context-menu.spec.tsx +++ b/services/web/test/frontend/features/source-editor/components/codemirror-editor-context-menu.spec.tsx @@ -65,6 +65,34 @@ const MockFileTreeDataProvider: FC = ({ value={ { selectedEntities: [{ type: 'doc', id: '_root_doc_id' } as any], + fileTreeData: { + _id: 'root-folder-id', + name: 'rootFolder', + docs: [{ _id: docId, name: 'test.tex' }], + folders: [], + fileRefs: [], + }, + } as any + } + > + {children} + +) + +const MockEmptyFileTreeDataProvider: FC = ({ + children, +}) => ( + @@ -1008,6 +1036,46 @@ describe('editor context menu', { scrollBehavior: false }, function () { }) }) + it('should close the menu and show an error toast when there is no resolvable file path', function () { + const scope = mockScope() + + cy.intercept( + 'GET', + '/project/*/sync/code*', + cy.spy().as('syncToPdfRequest') + ) + + cy.mount( + + + + + + + ) + + cy.get('.cm-line').eq(10).rightclick() + + cy.findByRole('menu').within(() => { + cy.findByRole('menuitem', { name: /jump to location in pdf/i }).click() + }) + + cy.findByRole('menu').should('not.exist') + + cy.get('.global-toasts').should( + 'contain.text', + 'That didn’t work. Try switching files and try again.' + ) + + cy.get('@syncToPdfRequest').should('not.have.been.called') + }) + it('should hide button when visual preview is enabled', function () { window.metaAttributesCache.set('ol-splitTestVariants', { 'editor-context-menu': 'enabled', diff --git a/yarn.lock b/yarn.lock index 6f44ae6fec..c5e3a68c5e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7870,6 +7870,7 @@ __metadata: version: 0.0.0-use.local resolution: "@overleaf/validation-tools@workspace:libraries/validation-tools" dependencies: + "@overleaf/logger": "workspace:*" "@overleaf/o-error": "workspace:*" "@types/express": "npm:4.17.23" "@types/node": "npm:^24.5.2"