Fix LSP path-traversal guard gaps, key-space diamond scope loss, and content-model tag desync#101
Merged
Merged
Conversation
…content-model tag desync Code review of the LSP server surfaced three correctness/security issues, each fixed with regression tests: - Path-traversal guard was only applied in keySpaceService.ts; hover, completion, definition, cross-reference validation, circular-reference detection, and the context-graph handler resolved user-authored href values without checking they stayed inside the workspace. Extracted the guard to a shared isPathWithinWorkspace() in textUtils.ts and applied it at every unguarded resolve+read/readdir site. - keySpaceService's BFS deduplicated visited maps by path only, so a submap reached via two different mapref @keyscope values only had its keys registered under the first scope. Cached each map's own key definitions on first visit and register them under later scope encounters too. - contentModelValidation's element-stack tracker unconditionally popped on a mismatched closing tag, desyncing the tree for the rest of the document. Now resyncs to the nearest matching ancestor, or leaves the stack untouched for a stray closing tag with no ancestor match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
… remaining path-traversal/root-id gaps Second review pass on the LSP server surfaced 5 more issues, each fixed with regression tests: - fragmentValidator.ts used getGlobalSettings() instead of getDocumentSettings(contextUri). globalSettings only gets refreshed when the client lacks configuration-pull capability, so dita/validateFragment (used by the AI Quick Fix feature to check LLM-generated fixes) silently validated against hardcoded defaults instead of the user's real customRulesFile/ditaRulesCategories/severityOverrides/etc. - wrapFragment()'s 'map' branch didn't wrap bare content in <map>...</map> when the fragment wasn't already map-rooted, so a fragment of sibling topicrefs became non-well-formed multi-root XML and was reported invalid even though the content itself was fine. - symbols.ts (both the DocumentSymbol tree builder and the flat workspace-symbol extractor) had the same mismatched-closing-tag bug fixed earlier in contentModelValidation.ts: on a stray/mismatched closing tag it removed only the matched stack entry instead of closing every intervening entry above it, corrupting Outline nesting and workspace-symbol containerName for the rest of the file. - documentLinks.ts resolved href/conref/xref/link targets without the isPathWithinWorkspace guard applied to sibling href-resolution sites in the first review pass, so a reference escaping the workspace produced a clickable link outside it. - workspaceValidation.ts's extractRootId matched the first id attribute anywhere in a file rather than specifically the root element, so a topic missing a root id but containing a nested child with one could be misindexed under that child's id for cross-file duplicate-ID detection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
Third review pass surfaced two more issues, both fixed with regression tests: - folding.ts had the same mismatched-closing-tag stack bug already fixed in contentModelValidation.ts and symbols.ts: on a stray/mismatched closing tag it removed only the matched stack entry instead of also closing every intervening never-closed ancestor above it. A later closing tag with the same name as one of those stuck ancestors could then pair with it and produce a bogus fold range spanning unrelated content. - handleRename rewrote every conkeyref anywhere in the workspace that textually matched the renamed id's trailing segment, without checking that the conkeyref's key actually resolves (via the key space) to the file being renamed. Accepting a rename could silently corrupt an unrelated file's conkeyref just because it happened to reference a same-named id in a different document. Same-file href/conref refs had an analogous gap: only cross-file refs were filtered by resolved target path, so a same-file href pointing at a *different* file's element with a matching id text was rewritten too. handleRename is now async and takes an optional KeySpaceService to verify conkeyref targets before rewriting; when unavailable, conkeyref edits are skipped rather than risking a wrong rewrite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
Extends the fix applied to handleRename to the read-only Find All
References path, which had the identical gap:
- findCrossFileReferences (workspaceScanner.ts) matched conkeyref anywhere
in the workspace by element-ID text alone ("cannot resolve key
synchronously"), so an unrelated file's conkeyref referencing a
different key that happened to target a same-named id would show up as
a false-positive reference. Now async and, when a KeySpaceService is
supplied, verifies the key actually resolves to the target file before
including a conkeyref match; without one, conkeyref matches are excluded
rather than reported speculatively.
- handleReferences (references.ts) had the same gap for same-file
href/conref/conkeyref matches — only cross-file refs were filtered by
resolved target path. Same-file matches are now filtered the same way.
Both handleReferences and findCrossFileReferences are now async; callers
updated accordingly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
… and stale cross-file diagnostics Fourth review pass surfaced three more issues, each fixed: - completion.ts's findParentElement() had the same mismatched-closing-tag stack bug found and fixed four times already (contentModelValidation.ts, symbols.ts x2, folding.ts): it removed only the matched stack entry instead of every intervening never-closed ancestor above it, so element completions after a malformed document could offer children of the wrong (stale) parent element. Regression test added. - settings.ts's getDocumentSettings() cached the in-flight connection.workspace.getConfiguration() promise with no rejection handler. A single transient failure (client error, timeout) permanently poisoned that resource's cache entry with a forever-rejected promise, silently breaking all validation for that document until an unrelated configuration-change event cleared the whole cache. Now evicts the entry and falls back to defaults on failure so the next call retries. Regression test added. - server.ts's onDidChangeWatchedFiles handler only invalidated the cross-reference diagnostic cache for all open documents when a .ditamap/ .bookmap changed (classification.mapChanged). Saving a plain .dita topic file (e.g. renaming or removing an element id) left other open documents' cached DITA-XREF-*/DITA-KEY-* diagnostics stale until they were independently edited. Broadened the condition to classification.ditaFileChanged (true for any .dita/.ditamap/.bookmap change; mapChanged is a strict subset of it), so any external DITA file change now triggers the cross-document revalidation it always should have. (documents.onDidChangeContent's live in-memory edit path is left map-only by design — cross-ref validation is intentionally save- triggered, not edit-triggered, to avoid I/O-heavy revalidation storms on every keystroke.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
…n content-model gaps Fifth review pass surfaced two more issues, both fixed with regression tests: - fixDeprecatedAltAttr (codeActions.ts) computed the enclosing <image> tag by searching *forward* from the diagnostic's range start. But the DITA-SCH-011 diagnostic (ditaRulesValidator.ts) points at the alt="..." attribute itself, not the <image tag start, so the forward search always skipped past the diagnosed tag and matched the *next* <image> element in the document instead — silently converting an unrelated later image's alt attribute while leaving the one the user asked to fix untouched (and producing garbage output with no later image existed to accidentally "rescue" it). Existing tests didn't catch this because they constructed diagnostics with range.start at offset 0, not the attribute's real location. Fixed to search backward for the enclosing tag from the diagnostic offset; test diagnostics now mirror the real validator's range, plus a two-image regression test. - contentModelValidation.ts's 'body', 'conbody', and 'section' content models were missing 'parml', 'screen', and 'syntaxdiagram' (conbody: just 'screen') from their allowed-children lists, even though ditaSchema.ts's DITA_ELEMENTS — which drives element completion for the same parents — lists all of them as valid. Accepting the tool's own autocomplete suggestion for any of these elements immediately produced a false-positive DITA-CM-001/003 diagnostic. Added the missing entries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
… and cross-file matching CI on windows-latest (PR #101) failed 4 tests added in this branch: handleDocumentLinks, handleReferences, and two handleRename cases. All four compare a path derived via uriToPath() (a file:// URI round-trip, which vscode-uri lowercases the Windows drive letter of) against a path from a different source that keeps its original case — e.g. a raw fs.mkdtempSync() workspace folder, or a KeySpaceService.resolveKey() targetFile. isPathWithinWorkspace() and the conkeyref/href file-part comparisons in rename.ts, references.ts, and workspaceScanner.ts all used a bare path.normalize(), which does not fold case, so on Windows (whose filesystem is case-insensitive) two paths naming the same file compared as different. textUtils.ts already has normalizeFsPath() for exactly this — it lowercases on win32 — but isPathWithinWorkspace() (added in the first review round) and the newer cross-file matching helpers didn't use it. Switched all of these path-identity comparisons to normalizeFsPath(). Added a portable regression test (mocks process.platform rather than relying on an actual Windows path) that reproduces the case mismatch and confirms isPathWithinWorkspace now tolerates it on win32 while remaining case-sensitive elsewhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
handleGetContextGraph was called without keySpaceService, so the tool's map-structure traversal never checked isPathWithinWorkspace and could resolve topicrefs that escape the workspace root. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
…ound isPathWithinWorkspace() was being applied to every relative reference a document makes, even when the document itself lives entirely outside the configured workspace folders (e.g. a loose file opened via File > Open). That blocked same-directory sibling references with no workspace boundary to actually enforce. Add effectiveWorkspaceFolders(), which falls back to permissive (single-file) mode when the source document isn't inside any workspace folder, and use it in hover, completion, definition, crossRefValidation, contextGraph, documentLinks, and validationPipeline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
When a submap was reached a second time via a different @keyscope chain (a diamond-shaped map graph), buildKeySpace() registered the submap's own direct keys under the new scope but never re-queued its children, so any keys defined further down that submap's tree were missing from the second scope entirely. Track submaps (with their keyscopes/inline keys) in mapDirectKeysCache and re-queue them under the newly combined scope prefixes on revisit. Guard against infinite requeueing on cyclic map graphs by recording, per map, the set of scope-prefix signatures already registered, and skipping a revisit once its exact signature has been seen. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
resolveKey/explainKey's topicToScope lookup, the buildKeySpace BFS visited-set key, extractTopicReferences' topicToScope population, and doInvalidate's cache-busting comparison all used bare path.normalize(), which does not fold case. vscode-uri's URI.file() lowercases Windows drive letters on round-trip, so a context path derived from a document URI could differ in case from the same path as resolved via path.resolve() while walking a map — silently missing scope-aware key lookups and cache invalidation on Windows. Switch all of them to normalizeFsPath(), which already handles this case-folding consistently elsewhere in the codebase. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
Rename and Find All References intentionally skip conkeyref matches they can't verify against a KeySpaceService, to avoid rewriting or reporting an unrelated file whose element merely shares the same id text — that behavior is correct and stays unchanged. But the skip was silent, so a user in single-file mode (or any other context missing a KeySpaceService) would see an incomplete rename/reference result with no indication anything was left out. Thread an optional log callback through collectMatchingEdits (rename), filterMatchingRefs (references), and findCrossFileReferences (workspaceScanner), wired to connection.console.warn in server.ts, so each skipped conkeyref is now visible in the server log. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
The same bug class — a closing tag matching an entry deeper in a name-keyed stack, where popping only that entry leaves stale intervening ancestors behind and desyncs every later lookup — had been independently fixed five times (contentModelValidation.ts, symbols.ts twice, folding.ts, completion.ts), each with its own hand-rolled top-down search-and-truncate loop. Add resyncStackToMatch() in utils/tagStack.ts: a generic search + truncate-through-match primitive with an optional per-discarded-entry callback for call sites that need to finalize or read matched/ intervening entries before they're dropped (symbols.ts's range/children finalization, folding.ts's fold-range emission). Use it at all five call sites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
rename.ts's collectMatchingEdits, references.ts's filterMatchingRefs, and workspaceScanner.ts's findCrossFileReferences each carried their own copy of the same href/conref/conkeyref target-matching logic (including the conkeyref-without-KeySpaceService skip-and-log added in the previous fix). Divergence between the three was already starting to show in slightly different log message wording. Add referenceMatchesTarget() in workspaceScanner.ts and use it at all three call sites, so the matching rules — and their logging — stay in one place. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
findCrossFileReferences (workspaceScanner.ts) and handleRename's cross-file loop / collectMatchingEdits (rename.ts) awaited keySpaceService.resolveKey() one ref, one file, at a time — on a workspace with many conkeyref references this serializes a lot of independent key-space lookups that could run concurrently. KeySpaceService already dedupes concurrent buildKeySpace calls for the same root map via its pendingBuilds map, so this is safe. Restructure both to resolve refs within a file (and files across the workspace) via Promise.all, which also preserves the original sequential ordering of results since Promise.all resolves in input order regardless of completion order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
…alize Three tests queried keySpace.topicToScope with path.normalize(guidePath) directly, but the Windows path-case fix switched the production code that populates topicToScope to normalizeFsPath() (which lowercases on win32). On windows-latest CI the stored key was lowercased while the test's lookup key wasn't, so the Map.get() missed and these tests failed: - "topicToScope maps topic to its owning scope prefix" - "hrefs inside reltable do not pollute topicToScope" - "topic inside inline scope branch is recorded in topicToScope" Switch all three lookups to normalizeFsPath() to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
jyjeanne
pushed a commit
that referenced
this pull request
Jul 5, 2026
… and cross-file matching CI on windows-latest (PR #101) failed 4 tests added in this branch: handleDocumentLinks, handleReferences, and two handleRename cases. All four compare a path derived via uriToPath() (a file:// URI round-trip, which vscode-uri lowercases the Windows drive letter of) against a path from a different source that keeps its original case — e.g. a raw fs.mkdtempSync() workspace folder, or a KeySpaceService.resolveKey() targetFile. isPathWithinWorkspace() and the conkeyref/href file-part comparisons in rename.ts, references.ts, and workspaceScanner.ts all used a bare path.normalize(), which does not fold case, so on Windows (whose filesystem is case-insensitive) two paths naming the same file compared as different. textUtils.ts already has normalizeFsPath() for exactly this — it lowercases on win32 — but isPathWithinWorkspace() (added in the first review round) and the newer cross-file matching helpers didn't use it. Switched all of these path-identity comparisons to normalizeFsPath(). Added a portable regression test (mocks process.platform rather than relying on an actual Windows path) that reproduces the case mismatch and confirms isPathWithinWorkspace now tolerates it on win32 while remaining case-sensitive elsewhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Code review of the LSP server surfaced three correctness/security issues,
each fixed with regression tests:
completion, definition, cross-reference validation, circular-reference
detection, and the context-graph handler resolved user-authored href
values without checking they stayed inside the workspace. Extracted the
guard to a shared isPathWithinWorkspace() in textUtils.ts and applied it
at every unguarded resolve+read/readdir site.
reached via two different mapref @keyscope values only had its keys
registered under the first scope. Cached each map's own key definitions
on first visit and register them under later scope encounters too.
a mismatched closing tag, desyncing the tree for the rest of the
document. Now resyncs to the nearest matching ancestor, or leaves the
stack untouched for a stray closing tag with no ancestor match.
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01QStkobe5KPqky2KYQz2axj