[michaelthyregod]

Not benchmarked yet: I'll run the benchmarks once my pending AngleSharp core change (part of an AngleSharp.Css fix) is settled.

Reworked as discussed: the external DTD subset now loads through the AngleSharp ecosystem instead of the file system.

Problem

Parsing read the file a document's <!DOCTYPE r SYSTEM "..."> named with File.ReadAllText, from any path, with default options. That includes UNC paths: \\server\share\x.dtd makes Windows connect to server and authenticate as the current user while the document is parsed.

Change

• Fetched like the document's other resources: through the browsing context's IResourceLoader, which only exists when resource loading is enabled, and its requesters, which decide the protocols.
  • Local files need a file: requester, such as AngleSharp.Io's.
  • Relative SYSTEM identifiers resolve against the document URL.
  • Without a loader, nothing is loaded, which is the default for new XmlParser() and WithXml() alone.
  • The ResourceRequest needs an element for its origin and referrer. The DOCTYPE comes before any element, so a detached element of the document stands in.
• One loading path: XmlDomBuilder.LoadExternalSubsetAsync. The async parse awaits it (ConfigureAwait(false)), and the sync parse waits for it. The docs point callers on a synchronization context to the async parse.
• The direct file access is removed: TryLoadExternalSubset and TryResolveSystemPath in the builder, and DtdParser's GetText / TryResolvePath.
  • External entities are no longer read; they get no replacement text.
  • DtdParser currently fails on the first token of every subset (swallowed by the builder), so that path didn't run, but it would have read any file a document named once DtdParser worked.
• Docs: 04-Questions, 05-DTD-Validation (new section "Loading the external subset"), AGENTS.md and the CHANGELOG are updated.

Tests

• Mock requester: Mocks/MockRequester serves mock: and http://*.test resources. Registered as the only requester, so WithDefaultLoader adds no HTTP requester and no test reaches the network.
• XmlExternalDtdSupport covers:
  • validation and entities from the external subset, sync and async;
  • relative identifiers against the document URL;
  • nothing loaded without resource loading or without a loader;
  • a local file path is not read without a file: requester;
  • an unloadable subset is ignored.
• The two other external DTD tests (XmlDeepNesting, XmlDtdIdentityValidation) serve their DTD through the mock instead of a temp file. Their documents and assertions are unchanged.
• Benchmarks: the External rows load their DTD through a small file: requester, and GlobalSetup checks that the DTD is applied.
• 627 tests pass.

🤖 Generated with Claude Code

[FlorianRappl]

Too many changes.

[michaelthyregod]

#38 is merged now and this is rebased onto devel, so it only shows its own changes: 2 commits, 14 files (+281/-24). Most of it is docs and tests; happy to split the docs into a separate PR if that helps.

[FlorianRappl]

Does not look like a good solution to me.

A good solution uses the AngleSharp ecosystem and it's requesters. So local files can only be read is there is a requested for file: protocols (eg as provided by AngleSharp.Io). Same is true for URLs etc

This way one can be really flexible up to the point of mocking input or determining the resolution on the fly.

[michaelthyregod]

Makes sense, I'll rework it that way:

• Resolve the SYSTEM identifier against the document URL and fetch it through the browsing context's resource loader and requesters. Without a loader (or without a requester for the protocol) nothing is loaded, so local files only load with a file: requester such as AngleSharp.Io's, and tests can use a mock requester.
• This replaces IsResolvingExternalEntities and the direct file access (LocalFiles included); keeping UNC paths out becomes the requester's job.

One question: the requesters are async. Should external DTDs only load in ParseDocumentAsync and browsing-context loading, with the synchronous ParseDocument ignoring them (rather than blocking on the requester)? That's what I'd do.

Also worth noting: an app that configures WithDefaultLoader for pages would then also fetch DTDs over HTTP from URLs the document chooses. Should that follow LoaderOptions.IsResourceLoadingEnabled, or is the configured requester enough?

[FlorianRappl]

Well from the inside we don't know/don't care of we have been called async - so I would not differentiate.

Regarding the loading: true I'd only call this if resource loading is enabled. Makes most sense to me.

[michaelthyregod]

Thanks. Loading only when resource loading is enabled means going through the browsing context's IResourceLoader, which only exists then. That's what I'll do.

On sync vs. async: the requesters only offer Task-based APIs. To load the DTD in the synchronous ParseDocument, the parser would have to block on that task (GetAwaiter().GetResult()). That ties up the thread for the whole download, and it can deadlock when the caller runs under a synchronization context (UI threads, classic ASP.NET), if the requester resumes on that context.

Would you accept blocking there, or rather that the synchronous parse skips external DTDs (or only uses them when the parse runs through the async path, e.g. browsing-context loading)? I'll implement whichever you prefer.

[FlorianRappl]

Someone who uses the sync path (not the recommendation but it's fine) should not use requesters - and if should not put them into the same synchronization context. From my perspective this is on the user.

I'd not distinguish here and I'd use the same logic for both ways (there should not be a duplicate).

[michaelthyregod]

Reworked as discussed: the external subset now loads through IResourceLoader (only with resource loading enabled) and its requesters, with one loading path that the sync parse waits for. Direct file access is gone, including in DtdParser.

[michaelthyregod]

Thanks. The benchmark's Documents.cs is back to the original path (no Uri), and the CHANGELOG entry is shortened and uses "Updated" (1c75fa7).

On interrupting the parse: it waits at the DOCTYPE because entities declared in the external subset (<!ENTITY greet ...>) are replaced while the body is tokenized. If the subset arrives later, &greet; is an unknown entity, and strict parsing throws. Validation alone could wait until the end. Two ways to avoid the interruption:

• (b) Start the download at the DOCTYPE, keep parsing, and wait only at the end for validation. Nothing interrupts the parse, but entities from the external subset stop working (today they're supported and tested).
• (c) Start the download at the DOCTYPE and wait only when it's needed: when the body references an entity that isn't declared internally, otherwise at the end for validation. Entities keep working, and most documents are never interrupted; it's a bit more code.

Which do you prefer? And should this still go into 1.3.0 (#43), or after it?

[FlorianRappl]

LGTM!

[FlorianRappl]

Good point regarding the entity validation. Can we aggregate the entities and defer their validation / resolution to the end of the parse? This way, we can keep on parsing and only deal with the validation as whole at the end.

[michaelthyregod]

Thanks! Yes, that's doable:

1. At the DOCTYPE the download starts and parsing continues.
2. A reference to an entity that isn't declared internally doesn't throw while a download is pending. It's kept, and the parser records where it sits (text node or attribute, and offset).
3. At the end, the parse waits for the external subset, registers its entities, and replaces the recorded references.
4. Then validation runs as now; in strict mode an entity that is still unknown throws then, pointing at the recorded position.

The replacement uses the recorded positions rather than searching the text afterwards, which would also hit escaped text such as &name;.

Since this PR is approved as it is, I'd suggest merging it and doing the deferral as a follow-up PR. Or would you rather have it in this one?

[FlorianRappl]

I'd see it in this one. Also note for (2): To not overcomplicate things I'd always do the entity resolution at the end. This way, it's consistent and we don't need to check if a download is active or finished. We always do the same.

[michaelthyregod]

Done in 07d4409: entity references are now always resolved at the end of the parse.

• At the DOCTYPE the external subset starts loading and the parse goes on. The tokenizer keeps every reference to a non-built-in entity as written and records its offset; the builder records the text node or attribute it lands in.
• At the end, both paths complete the same way: wait for the download, build the DTD, replace the recorded references by offset, then validate.
• Side effects, both fixes:
  • escaped &e; stays &e; (it was expanded as well when e was declared);
  • an empty entity now resolves to nothing (strict parsing threw on it).
• Unknown entities throw the same error at the same position, just at the end. OnCreated sees attribute values with their references as written; that's in the docs.

Tests: 635 pass, 8 new. One test holds the DTD back and checks that the whole body is parsed before it arrives. A 60,000-result differential fuzz against the previous commit differs only for escaped references and empty entities. Still not benchmarked.